Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

src: remove unused function #9243

Closed
wants to merge 1 commit into from

Conversation

mscdex
Copy link
Contributor

@mscdex mscdex commented Oct 23, 2016

Checklist
  • make -j8 test (UNIX), or vcbuild test nosign (Windows) passes
  • commit message follows commit guidelines
Affected core subsystem(s)
  • src
Description of change

Remove an unused function.

CI: https://ci.nodejs.org/job/node-test-pull-request/4627/

/cc @nodejs/v8-inspector

@mscdex mscdex added c++ Issues and PRs that require attention from people who are familiar with C++. inspector Issues and PRs related to the V8 inspector protocol labels Oct 23, 2016
@nodejs-github-bot nodejs-github-bot added the c++ Issues and PRs that require attention from people who are familiar with C++. label Oct 23, 2016
@bnoordhuis
Copy link
Member

See #9220 for an alternative take. Whether it's unused or not depends on whether the C++ standard library exports a std::to_string function.

@mscdex
Copy link
Contributor Author

mscdex commented Oct 23, 2016

@bnoordhuis I can't seem to find what actually uses to_string() (whether it's built-in or not)? I can't find any references in src/, test/, or deps/v8_inspector/.

@bnoordhuis
Copy link
Member

Oh, it might be vestigial from the initial version of #8919. If it's not actually in use (and the CI seems to confirm that), then it LGTM.

Copy link
Member

@addaleax addaleax left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

jasnell pushed a commit that referenced this pull request Oct 25, 2016
PR-URL: #9243
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
@jasnell
Copy link
Member

jasnell commented Oct 25, 2016

Landed in ed41d8d

@jasnell jasnell closed this Oct 25, 2016
@mscdex mscdex deleted the inspector-remove-unused-func branch October 25, 2016 22:19
evanlucas pushed a commit that referenced this pull request Nov 3, 2016
PR-URL: #9243
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
MylesBorins pushed a commit that referenced this pull request Nov 18, 2016
PR-URL: #9243
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
MylesBorins pushed a commit that referenced this pull request Nov 19, 2016
PR-URL: #9243
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
MylesBorins pushed a commit that referenced this pull request Nov 22, 2016
PR-URL: #9243
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
@MylesBorins MylesBorins mentioned this pull request Nov 22, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c++ Issues and PRs that require attention from people who are familiar with C++. inspector Issues and PRs related to the V8 inspector protocol
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants